-
Notifications
You must be signed in to change notification settings - Fork 9.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
devtools: import html generator code #7421
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems like we should just try to make report-generator more easily consumable by devtools rather than create this console.log'd file.
were there particular challenges there that prevented us from taking that route?
console.log('const htmlReportAssets =', JSON.stringify(htmlReportAssets, null, 2), ';'); | ||
console.log('class ReportGenerator {'); | ||
console.log(' static Assets = htmlReportAssets;'); | ||
console.log(' static', reportGenerator.generateReportHtml.toString()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this feels....fragile to say the least :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about bundling w/ browserify + this plugin? https://github.com/browserify/brfs
The file system access is what made it tricky to consume in DT.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not doing above, but I did a refactor. Changed the report generator module to load from Runtime.cachedAssets if the env is Devtools, and replaced the node bundling code with just copying those assets. Chromium CL not updated yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bonus - fewer bytes wasted from string escaping :)
cp -pPR $report_dir/{report-styles.css,templates.html,renderer} "$fe_lh_dir" | ||
cp -pPR $report_dir/report-generator.js "$fe_lh_dir" | ||
cp -pPR $report_dir/html/renderer "$fe_lh_dir"/renderer | ||
# import report assets |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
say "import report" 10 times quickly :)
Seems like this has a large overlap with what viewer has to do to save the HTML report. Is it possible we could share that between the two at build time rather than check at runtime? |
good idea, I'll see what could be done. |
At first glance, it seems like it'd be a large refactor and wouldn't be as simple to understand as the current code in |
we crafted report generation to work with devtools before, going through a few solutions until finding a nice one where we could just cut off the dependency tree at Some ideas:
const generatorFilename = `./lighthouse-core/report/report-generator.js`;
const generatorBrowserify = browserify(generatorFilename, {standalone: 'ReportGenerator'})
.transform('brfs'); (which puts
// clients/devtools-report-assets.js
'use strict';
module.exports = {
REPORT_TEMPLATE: Runtime.cachedResources['audits2/lighthouse/report-template.html'],
REPORT_TEMPLATES: Runtime.cachedResources['audits2/lighthouse/report-templates.html'],
REPORT_JAVASCRIPT: Runtime.cachedResources['audits2/lighthouse/report.js'],
REPORT_CSS: Runtime.cachedResources['audits2/lighthouse/report.css'],
}; and then have browserify insert the replacement by adding const pathToReportAssets = require.resolve('./clients/devtools-report-assets.js');
generatorBrowserify.require(pathToURLShim, {expose: './html/html-report-assets'}); |
Did this. 2) would be nice in terms of reducing bytes (would remove tons of escaping), but would require the build script to copy the assets. but should we bother with the extra complexity? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
method change question, but otherwise LGTM!
@@ -8,6 +8,10 @@ | |||
const htmlReportAssets = require('./html/html-report-assets'); | |||
|
|||
class ReportGenerator { | |||
static get Assets() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
static get Assets() { | |
static getAssets() { |
any specific reason to have it as a getter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i thought it looked better when used: https://chromium-review.googlesource.com/c/chromium/src/+/1510732/5/third_party/blink/renderer/devtools/front_end/audits2/Audits2Panel.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think they're usually just lying as an API--maybe justified if you're trying to match an expected interface--but we don't really have an established style on this front so to each their own :)
Should be lowercase assets
, though
echo -e "\033[32m ✓\033[39m Report renderer files copied." | ||
|
||
# copy lighthouse-dt-bundle (potentially stale) | ||
cp -pPR "$lh_bg_js" "$lh_worker_dir/lighthouse-dt-bundle.js" | ||
echo -e "\033[96m ✓\033[39m (Potentially stale) lighthouse-dt-bundle copied." | ||
|
||
# bundle report generator | ||
yarn browserify lighthouse-core/report/report-generator.js -o $fe_lh_dir/report-generator.js -s ReportGenerator -t brfs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it could be helpful to move this into a script in build/
and output to dist/dt-report-generator-bundle.js
(or whatever) and have roll-to-devtools.sh
only responsible for copying things over. That way all our client building is all in one place and the requirements (e.g. browserify compatible) are plain to see.
Buuuut, this is also so succinct, if you don't want to move on that today I'm ok with that too :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved to build/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so beautiful now! 😍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! l like this
default.profraw
accidentally committed?
.bundle((err, src) => { | ||
if (err) throw err; | ||
fs.writeFileSync(outFile, src.toString()); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sweet, I think this might help @exterkamp as well. At least I think he was looking for a standalone report generator?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool! He can just remove the dt-
bit then, that'd be a great PR @exterkamp :)
Changelog: GoogleChrome/lighthouse@v4.2.0...9790337 Relevant LH build changes: GoogleChrome/lighthouse#7421 GoogleChrome/lighthouse#7567 Bug: 772558 Change-Id: I50296da5059d6f90fbd69346691529a5feff15ad Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1510732 Commit-Queue: Connor Clark <cjamcl@google.com> Reviewed-by: Paul Irish <paulirish@chromium.org> Reviewed-by: Dmitry Gozman <dgozman@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#649003} Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src Cr-Mirrored-Commit: 1df082f1ff78ab6009a972d861537484f967d3d5
Changelog: GoogleChrome/lighthouse@v4.2.0...9790337 Relevant LH build changes: GoogleChrome/lighthouse#7421 GoogleChrome/lighthouse#7567 Bug: 772558 Change-Id: I50296da5059d6f90fbd69346691529a5feff15ad Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1510732 Commit-Queue: Connor Clark <cjamcl@google.com> Reviewed-by: Paul Irish <paulirish@chromium.org> Reviewed-by: Dmitry Gozman <dgozman@chromium.org> Cr-Commit-Position: refs/heads/master@{#649003}
Changelog: GoogleChrome/lighthouse@v4.2.0...9790337 Relevant LH build changes: GoogleChrome/lighthouse#7421 GoogleChrome/lighthouse#7567 Bug: 772558 Change-Id: I50296da5059d6f90fbd69346691529a5feff15ad Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1510732 Commit-Queue: Connor Clark <cjamcl@google.com> Reviewed-by: Paul Irish <paulirish@chromium.org> Reviewed-by: Dmitry Gozman <dgozman@chromium.org> Cr-Commit-Position: refs/heads/master@{#649003}
Bundle the HTML generation code. No longer import templates/stylesheets, as they are already in the new bundled fine.
Chromium changes